Add Results configuration in tekton config #2398
Conversation
|
Skipping CI for Draft Pull Request. |
tekton-robot
added
release-note
tekton-robot
added
the
size/L
|
@pratap0007 include the doc changes[1] on this PR |
pratap0007
force-pushed
the
default-results
branch
2 times, most recently
from
dbb72d6 to
42f99c8
Compare
pratap0007
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
pratap0007
force-pushed
the
default-results
branch
from
42f99c8 to
a1ce3b9
Compare
tekton-robot
added
size/XXL
|
The following is the coverage report on the affected files.
|
pratap0007
force-pushed
the
default-results
branch
2 times, most recently
from
1c5b5ff to
13655eb
Compare
tekton-robot
added
size/L
|
The following is the coverage report on the affected files.
|
pratap0007
force-pushed
the
default-results
branch
from
13655eb to
055a55b
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-operator-integration-tests |
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-operator-integration-tests |
2 similar comments
|
/test pull-tekton-operator-integration-tests |
|
/test pull-tekton-operator-integration-tests |
pratap0007
force-pushed
the
default-results
branch
from
d79866d to
00bbc52
Compare
|
The following is the coverage report on the affected files.
|
| tr.Status.MarkDependencyMissing(fmt.Sprintf("Default db %s creation is failing", DbSecretName)) | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
If it is another exception, such as a network timeout, should an error be returned here?
There was a problem hiding this comment.
Do you think writing it this way would be clearer?
if err != nil && apierrors.IsNotFound(err) {
_, err = r.kubeClientSet.CoreV1().Secrets(tr.Spec.TargetNamespace).Create(ctx, newDBSecret, metav1.CreateOptions{})
...
}
return errThere was a problem hiding this comment.
yes, would be clear if we have to check only a condition
There was a problem hiding this comment.
The current writing places a significant mental burden.
It is necessary to consider whether the final return nil is accurate.
Would this be better?
if err == nil {
return nil
}
if !apierrors.IsNotFound(err) {
// logger
return err
}
newDBSecret := r.getDBSecret(DbSecretName, tr.Spec.TargetNamespace, tr)
...There was a problem hiding this comment.
yes, it would be better
| newDBSecret := r.getDBSecret(DbSecretName, tr.Spec.TargetNamespace, tr) | ||
| _, err := r.kubeClientSet.CoreV1().Secrets(tr.Spec.TargetNamespace).Create(ctx, newDBSecret, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| logger.Error(err) |
There was a problem hiding this comment.
Could you add some contextual information to the error here, so it's easier to identify where the error was triggered when viewing the logs?
There was a problem hiding this comment.
@pratap0007 Please address the review comments, It looks fine.
pratap0007
force-pushed
the
default-results
branch
from
00bbc52 to
0260ad4
Compare
|
The following is the coverage report on the affected files.
|
|
|
||
| _, err := r.kubeClientSet.CoreV1().Secrets(tr.Spec.TargetNamespace).Create(ctx, secret, metav1.CreateOptions{}) | ||
| if err != nil { | ||
| logger.Error(err) |
| tr.Status.MarkDependencyMissing(fmt.Sprintf("Default db %s creation is failing", DbSecretName)) | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
The current writing places a significant mental burden.
It is necessary to consider whether the final return nil is accurate.
Would this be better?
if err == nil {
return nil
}
if !apierrors.IsNotFound(err) {
// logger
return err
}
newDBSecret := r.getDBSecret(DbSecretName, tr.Spec.TargetNamespace, tr)
...
pratap0007
force-pushed
the
default-results
branch
from
0260ad4 to
70f1a26
Compare
|
The following is the coverage report on the affected files.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: l-qing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pratap0007
force-pushed
the
default-results
branch
from
70f1a26 to
71f5652
Compare
tekton-robot
added
the
ok-to-test
|
The following is the coverage report on the affected files.
|
pratap0007
force-pushed
the
default-results
branch
from
71f5652 to
7d696c2
Compare
|
The following is the coverage report on the affected files.
|
This will add the Results configuration to the tekton config and will be installed by default through tekton config Signed-off-by: Shiv Verma <[email protected]>
pratap0007
force-pushed
the
default-results
branch
from
7d696c2 to
00091f3
Compare
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-operator-integration-tests |
|
/retest |
1 similar comment
|
/retest |
installed by default through tekton config
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lintbefore submitting a PRSee the contribution guide for more details.
Release Notes